FEAT: Improve interactions of user pools - #1009
Conversation
ColmTalbot
added
>100 lines
refactoring
sampling
mj-will
left a comment
mj-will
left a comment
There was a problem hiding this comment.
Looks great, I look forward to trying it! I've added some initial comments but might need to have a second look.
ColmTalbot
left a comment
ColmTalbot
left a comment
There was a problem hiding this comment.
I don't remember exactly what the behaviour for some of these questions is, so I'll go back and check, and ideally write docstrings about them.
mj-will
left a comment
mj-will
left a comment
There was a problem hiding this comment.
Forgot to submit my comments as a review, but see them above.
adivijaykumar
left a comment
adivijaykumar
left a comment
There was a problem hiding this comment.
Can we add a test for schwimmbad/MPI support?
|
We should documentation for user pools and note that the pool will only work correctly in the initializer has been called. |
mj-will
left a comment
mj-will
left a comment
There was a problem hiding this comment.
Coming back to this again, I think the only pending item is documentation for the new pool support.
I haven't added an MPI test, but we have a schwimmbad test using the SerialPool, which should have the same API. |
FEAT: improve reweighting parallelisation FEAT: add parameters as argument to new pool BUG: test that pool exists at cleanup BUG: test pool exists at closing REFACTOR: refactor run_sampler to simplify pool logic DEP: discourage setting up pool in sampler REFACTOR: remove top level multiprocessing import BUG: make sure prior is passed to pool creation BUG: fix test failures TEST: fix reproducibility test BUG: fix a typo in conversion function test MAINT: don't create pool of size 1 BUG: only include chunksize in multiprocessing map DOC: add docstrings for pool functions DOC: update pool docstrings Address review comments TYPO: Fix typo in parameter description comments BUG: move definition of chunk size in reweighting MAINT: remove old function Fix wrong function name in docstring REFACTOR: refactor pool initialization and add documentation FMT: precommit fixes TST: fix failing tests
97c094f to
d9fe332
Compare
mj-will
left a comment
mj-will
left a comment
There was a problem hiding this comment.
Overall LGTM, just one question on the deprecation.
| if self.pool is not None: | ||
| logger.warning( | ||
| "Setting up parallel pool in sampler is deprecated. Use " | ||
| "bilby.utils.parallel.bilby_pool context instead." | ||
| ) |
There was a problem hiding this comment.
Does this mean we need to change how the pool is called in bilby_pipe?
There was a problem hiding this comment.
I can probably clarify this, as the call order is pretty confusing. This should only be triggered if people aren't using bilby.run_sampler, which is a pretty minimal subset of people who are directly invoking the sampler classes. In that case, the recommendation is to use the bilby_pool context.
If you have a suggestion for more clear text, I'd be open to it.
There was a problem hiding this comment.
Ah, that makes sense.
Perhaps clarifying which method is being called would help? Maybe something like:
| if self.pool is not None: | |
| logger.warning( | |
| "Setting up parallel pool in sampler is deprecated. Use " | |
| "bilby.utils.parallel.bilby_pool context instead." | |
| ) | |
| if self.pool is not None: | |
| logger.warning( | |
| "Setting up parallel pool in Sampler._setup_pool is deprecated. " | |
| "Use bilby.utils.parallel.bilby_pool context instead." | |
| ) |
| due to the use of global storage of the likelihood and priors. | ||
| However, we plan to remove this limitation in a future release. | ||
|
|
||
| Default parallelisation |
There was a problem hiding this comment.
I'm a bit confused by this given there's a deprecation warning above. I think I'm missing somethign about the deprecation but I'm not sure what.
3 participants
I noticed that it was difficult to pass a user-specified pool through run_sampler, and it isn't used at all in the post processing. This PR:
with multiprocessing.pool()...)Sampler._setup_poolandSampler._close_poolparallel_bilbymoot.